Introduce new icarReproSireInfoType, for the basic sire properties.#590
Conversation
|
[heart] Schultz, Andreas reacted to your message:
…________________________________
From: Alexey Shelepaev ***@***.***>
Sent: Wednesday, February 11, 2026 8:01:20 AM
To: adewg/ICAR ***@***.***>
Cc: Schultz, Andreas ***@***.***>; Mention ***@***.***>
Subject: [adewg/ICAR] Introduce new icarReproSireInfoType, for the basic sire properties. (PR #590)
Due to missing permissions in the adewg repository, I created the PR against Andrea's original branch (adewg:sirePrimaryBreed-in-insemination).
Could you please review the PR @cookeac<https://github.com/cookeac> @erwinspeybroeck<https://github.com/erwinspeybroeck> @AndreasSchultzGEA<https://github.com/AndreasSchultzGEA>?
Once it’s approved, we can proceed with merging the parent PR into Develop.
________________________________
You can view, comment on, or merge this pull request online at:
#590
Commit Summary
* fe2689c<fe2689c> Introduce new icarReproSireInfoType, for the basic sire properties.
File Changes
(5 files<https://github.com/adewg/ICAR/pull/590/files>)
* M resources/icarReproEmbryoResource.json<https://github.com/adewg/ICAR/pull/590/files#diff-05f0cb26c19d38b168c94cad1427c9e1616eadbd4d3913d0aaaa2dbb4badf5d6> (25)
* M resources/icarReproInseminationEventResource.json<https://github.com/adewg/ICAR/pull/590/files#diff-bd02c1edcb121e0ebdbb61f3cf4d5b8a6877a9751feb3c8884e410ecb9be48be> (25)
* M resources/icarReproSemenStrawResource.json<https://github.com/adewg/ICAR/pull/590/files#diff-a99caf53a5f963556efdf5b6a464df95804c0155a2dae3f7a4436919579b6d16> (25)
* A types/icarReproSireInfoType.json<https://github.com/adewg/ICAR/pull/590/files#diff-c307b272242915c042aab4e318676fd9fde53378b89c96cd4a3b5e7c19f526f4> (27)
* M types/icarSireRecommendationType.json<https://github.com/adewg/ICAR/pull/590/files#diff-d95eb11eaca281b4a6980a586d01e7730f5d688d98f6e32d19575e57718d0cd9> (64)
Patch Links:
* https://github.com/adewg/ICAR/pull/590.patch
* https://github.com/adewg/ICAR/pull/590.diff
—
Reply to this email directly, view it on GitHub<#590>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ATBWSFSEUYVEZZ4N45T2PNL4LLOVBAVCNFSM6AAAAACUW4TXA6VHI2DSMVQWIX3LMV43ASLTON2WKOZTHEZDKMRSHAYTCMA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
AndreasSchultzGEA
left a comment
There was a problem hiding this comment.
In general, the changes seem to be okay.
But an replacement will produce a breaking change.
Therefore, a rollout with 1.5.2 or 1.6.0 is not recommended.
| "type": "string", | ||
| "description": "URI to an AnimalCoreResource for the donor dam." | ||
| }, | ||
| "sireIdentifiers": { |
There was a problem hiding this comment.
If this properties are replaced, there will be a breaking change!
There was a problem hiding this comment.
@AndreasSchultzGEA Thanks for raising this! I totally get why moving the properties into a common type can look like a breaking change at first glance.
If we focus on the actual JSON payloads, nothing really changes. The same properties are still there, at the same level, with the same names and types. From the consumer side, the payload looks exactly the same as before.
So any previously generated code that expects these properties in each schema should continue to work without issues, since the serialized JSON structure hasn’t changed.
Where it might feel like a breaking change is on the code generation side, since the schema structure changes and generators will produce slightly different models. But both the old and the new generated code versions still support the same JSON payload structure. So in terms of the actual data contract, nothing breaks.
That’s why I’d consider this more of a schema refactoring than a real breaking change.
There was a problem hiding this comment.
I agree it is a schema refactoring and not a breaking change to the schema. But to Andreas' point, how substantial are the changes to the generated code in C# and Java? Because if developers have to change a lot of their code that references the modified classes, it is still a breaking change for them.
There was a problem hiding this comment.
Thanks for your very valuable comments.
To summarize, there are three levels of compatibility we need to consider:
-
JSON payload – non-breaking → minor version
The JSON payload remains unchanged — same fields, same structure — so the wire contract is fully backward compatible. -
Generated API client/server code – non-breaking → minor version
The generated API code continues to support the same payload, so from an API usage perspective it also remains backward compatible. -
Application internal logic that depends on generated models – potentially breaking → major version
If an application’s internal logic depends directly on the previously generated model structure (for example, specific class hierarchy, inheritance, database mappings, or integrations with other components), then regenerating the models from the updated schema could introduce breaking changes at compile time. In that scenario, the impact would be considered a major change for those consumers.
@AndreasSchultzGEA you are refering to the third level, right?
There was a problem hiding this comment.
@AndreasSchultzGEA yes, I fully agree that we should always keep this in mind. So, you propose to keep this refactoring ouside this minor update?
@cookeac I’m wondering what level of compatibility guarantee ICAR ADE itself is expected to provide. Have we formally defined this somewhere?
From my perspective, the scope of compatibility for ICAR ADE updates could be structured as follows:
- JSON payload: The external data structure must remain stable — same fields, same structure, same semantics.
- Spec-level compatibility: Schema validity must be preserved, including consistent required/optional rules, types, and constraints.
- Code generation: At minimum, commonly used generators (e.g., C#, Java) should continue to produce usable output that correctly accepts and produces the same payloads.
- Consumer internal logic coupling: The most challenging aspect is applications that tightly couple their internal logic to the generated model hierarchy (e.g., database mappings, reflection usage, inheritance assumptions, etc.).
While we should acknowledge this risk and try to minimize unnecessary disruption, it may not be realistic to block schema refactoring solely because some internal implementations depend on specific model structures.
There was a problem hiding this comment.
At our meetings today we agreed that this can be merged once we have some brief documentation to accompany the 1.6 release so that users know that if they regenerate their code from the schema, they may need to change how they reference fields in these objects.
There will not be a breaking change for users who are interacting with another service as the JSON itself is not changed.
|
The C# code generation also seems to work fine with the changes above. And the final JSON payload still looks the same in the generated openapi.json schema. |
|
We tested the Java generation and that worked fine. Maybe we didn't use the parts which could possibly be altered after generation of new classes. Our Implementation worked without changes. |
| "type": "string", | ||
| "description": "URI to an AnimalCoreResource for the donor dam." | ||
| }, | ||
| "sireIdentifiers": { |
There was a problem hiding this comment.
At our meetings today we agreed that this can be merged once we have some brief documentation to accompany the 1.6 release so that users know that if they regenerate their code from the schema, they may need to change how they reference fields in these objects.
There will not be a breaking change for users who are interacting with another service as the JSON itself is not changed.
Due to missing permissions in the adewg repository, I created the PR against Andrea's original branch (adewg:sirePrimaryBreed-in-insemination).
Could you please review the PR @cookeac @erwinspeybroeck @AndreasSchultzGEA?
Once it’s approved, we can proceed with merging the parent PR into Develop.